Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement loadGraphModelSync #6428

Merged

Conversation

mattsoulanille
Copy link
Member

@mattsoulanille mattsoulanille commented May 19, 2022

Add loadGraphModelSync, which loads a model syncrhonously from an IOHandlerSync. It currently only accepts an IOHandlerSync, and does not allow passing the model URL directly.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@mattsoulanille mattsoulanille force-pushed the load_graph_model_sync branch 2 times, most recently from 0a22d01 to d41214f Compare May 19, 2022 20:19
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding this API, and it looks great.
Just a comment on the API doc. thanks

Reviewable status: 0 of 1 approvals obtained (waiting on @jinjingforever and @mattsoulanille)


tfjs-converter/src/executor/graph_model.ts line 474 at r1 (raw file):

}

export function loadGraphModelSync(

can you add jsdoc for this method similar to loadGraphModel

@mattsoulanille mattsoulanille requested a review from pyu10055 May 19, 2022 20:46
Copy link
Member Author

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

Reviewable status: 0 of 1 approvals obtained (waiting on @jinjingforever and @pyu10055)


tfjs-converter/src/executor/graph_model.ts line 474 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

can you add jsdoc for this method similar to loadGraphModel

Totally forgot that. Thanks!

I removed the options parameter too, since it's unused (options are passed to the http io handler internally in GraphModel).

Copy link
Member Author

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @jinjingforever and @pyu10055)


tfjs-converter/src/executor/graph_model.ts line 474 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

Totally forgot that. Thanks!

I removed the options parameter too, since it's unused (options are passed to the http io handler internally in GraphModel).

I'll add examples to the jsdoc when I add the sync http IOHandlerSync. Without that, the examples would be very verbose.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 5 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @mattsoulanille and @pyu10055)


tfjs-converter/src/executor/graph_model.ts line 486 at r3 (raw file):

  if (modelSource == null) {
    throw new Error(
        'modelUrl in loadGraphModelSync() cannot be null. Please provide a ' +

modelSource instead of modelUrl, also applies to the next error message.

Add loadGraphModelSync, which loads a model syncrhonously from an IOHandlerSync. It currently only accepts an IOHandlerSync, and does not allow passing the model URL directly.
@mattsoulanille mattsoulanille force-pushed the load_graph_model_sync branch from cb6be48 to b3d429d Compare May 19, 2022 21:46
@mattsoulanille mattsoulanille merged commit 07af317 into tensorflow:master May 19, 2022
@mattsoulanille mattsoulanille deleted the load_graph_model_sync branch May 19, 2022 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants